-
Notifications
You must be signed in to change notification settings - Fork 65
feat: mint + burn routing logic #1864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: mint + burn routing logic #1864
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some remarks! Also some inline comments seem to be incorrect/outdated
const isLargeDeposit = depositAmountUsd > 1_000_000; // 1M USD | ||
|
||
// Check if eligible for Fast CCTP (Polygon, BSC, Solana) and deposit > 10K USD | ||
const fastCctpChains = [CHAIN_IDs.POLYGON, CHAIN_IDs.BSC, CHAIN_IDs.SOLANA]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of the CCTP bridge strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for this use case we are already exporting CCTP_FILL_TIME_ESTIMATES
from the strategy. Maybe we can use that combined with a threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast CCTP chains can be taken from here. Chains with "seconds" are considered fast
a8c41f2
to
671465d
Compare
if (bridgeStrategyData.isUtilizationHigh) { | ||
return getCctpBridgeStrategy(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when utilization is high CCTP strategy should be used only for USDC, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we won't ever reach this line unless we're bridging USDC to USDC. See line 65 for the early exit clause.
|
||
import { CrossSwap, CrossSwapQuotes, Token } from "../_dexes/types"; | ||
import { AppFee, CrossSwapType } from "../_dexes/utils"; | ||
import { Logger } from "@across-protocol/sdk/dist/types/relayFeeCalculator"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have some utils in api/_logger.ts
. Can we use getLogger
from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I checked and that's also using the same one: https://github.com/across-protocol/frontend/blob/master/api/_logger.ts#L8
In this file, I'm only using the type Logger
, not the actual logger instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nice. Left some comments/questions.
return getAcrossBridgeStrategy(); | ||
} else { | ||
// Use OFT bridge if not CCTP | ||
return getCctpBridgeStrategy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all these checks, should we check if the strategy we want to return is part of the supportedBridgeStrategies
list? That way we avoid returning here a strategy that was previously filtered out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored it a bit to achieve this. It will help us when we add the OFT strategy as well.
api/_bridges/utils.ts
Outdated
const isFastCctpChain = | ||
fastCctpChains.includes(inputToken.chainId) || | ||
fastCctpChains.includes(outputToken.chainId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only the origin chain matters here.
api/_bridges/utils.ts
Outdated
const utilizationThreshold = sdk.utils.fixedPointAdjustment.mul(80).div(100); // 80% | ||
|
||
// Calculate current utilization percentage | ||
const currentUtilization = _utilizedReserves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think utilizedReserves can be negative, would that have any impact in this calculation?
I see in other places we floored it to zero when that happens. See for example: https://github.com/across-protocol/frontend/blob/master/api/_utils.ts#L2709
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
}); | ||
|
||
// Safely return undefined if we can't fetch bridge strategy data | ||
return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could instead return the expected JSON object but all fields set to false?
This would be a more clear return type than undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point, that's how I initially had it but decided to give a nullable return type instead because:
- We can only return all the fields populated in the JSON or none
- It's easier to check if we successfully fetched bridge strategy data or not. Example in these lines: https://github.com/across-protocol/frontend/pull/1864/files#diff-4c71009b75dec65194d6c79bf20fbd7122b0acfc372a2c946e8ac64de2cea414R59-R61
I think it makes sense to keep each field nullable like you said if we decide to support partial fetching of bridge strategy data, where none, some, or all of the fields can nullable. Let me know what you think though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we show this behaviour in the JS doc string of the function? As a user of this function it is not immediately clear what undefined means here. Undefined can mean anything if the function does not explicitly state its meaning. And then we have to be careful, that the function can never return undefined at any other point of the function as it now has a distinct meaning.
I'll leave the decision up to you. Usually, I would argue it makes sense to make things either intuitive/the code is documentation enough or specify the behaviour as stringently as possible to avoid ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can add some docs. My reasoning is that each of the fields individually should never be undefined. We either have the bridge strategy data or we don't - undefined just means that we don't.
If we make all the fields in the type nullable, the structure technically feels wrong. Another option is to take the undefined
out of the type and keep it in the function return type.
api/_bridges/utils.ts
Outdated
const depositAmountUsd = parseFloat( | ||
ethers.utils.formatUnits(amountInInputTokenDecimals, inputToken.decimals) | ||
); | ||
const isInThreshold = depositAmountUsd <= 10_000; // 10K USD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to consider extracted hardcoded variables into a single place. This would help us later to make changes or understand why certain thresholds were set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, will amend.
671465d
to
4493018
Compare
d26576e
to
e5c7a26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice this looks good!
This PR implements the routing logic below. Changes here: a8c41f2
/limits
to return utilization data so that we don't need to refetch itlimits
with a standard amount to maximize cache hits